Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Dashboard] Remove gpustats dependencies from Ray[default] #41044

Merged
merged 9 commits into from
Nov 22, 2023
Merged

[Dashboard] Remove gpustats dependencies from Ray[default] #41044

merged 9 commits into from
Nov 22, 2023

Conversation

jonathan-anyscale
Copy link
Contributor

@jonathan-anyscale jonathan-anyscale commented Nov 9, 2023 •
edited
Loading

Why are these changes needed?

Add method to get gpu utilization similarly on how gpustats did, and remove gpustats from ray[default] dependencies.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@jonathan-anyscale jonathan-anyscale mentioned this pull request Nov 9, 2023
Merged
8 tasks
@jonathan-anyscale
remove gpustats from ray default
13fb433 
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
@jonathan-anyscale jonathan-anyscale marked this pull request as ready for review November 13, 2023 22:52
@jonathan-anyscale
fix type class
48056cf 
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
jjyao
jjyao reviewed Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@jjyao jjyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lg. Could you update the PR description?

dashboard/client/src/pages/node/index.tsx Outdated Show resolved Hide resolved
dashboard/modules/reporter/reporter_agent.py Outdated Show resolved Hide resolved
dashboard/modules/reporter/reporter_agent.py Outdated Show resolved Hide resolved
@jonathan-anyscale
fix redundant
7d3c486 
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
@jjyao jjyao requested a review from wookayin November 15, 2023 16:51
@jonathan-anyscale
lint
b035611 
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
@jonathan-anyscale
nit fix
91e9725 
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
@jjyao
Copy link
Collaborator

jjyao commented Nov 16, 2023

@ericl needs your approval here as code owner.

Context: we are removing GPUtil and gpustats dependencies with a vendored in pyvnml single file library (the offical nvidia library that other libraries use internally) so that it works with minimal ray and it also removes unnecessary transitive dependencies included by gpustats for terminal display.

@jjyao jjyao requested a review from wookayin November 16, 2023 00:59
@jonathan-anyscale
downgrade nvidia-ml-py to 11.510.69, using v2 api
f23603d 
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
XuehaiPan
XuehaiPan reviewed Nov 17, 2023
View reviewed changes
dashboard/modules/reporter/reporter_agent.py Outdated
Comment on lines 441 to 450
memory_used=int(pynvml.nvmlDeviceGetMemoryInfo(gpu_handle).used)
// MB,
memory_total=int(pynvml.nvmlDeviceGetMemoryInfo(gpu_handle).total)
// MB,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could merge these two nvmlDeviceGetMemoryInfo calls into one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, in NVIDIA driver 510.39.01, a v2 memory info API was added:

https://github.com/NVIDIA/nvidia-settings/blob/510.39.01/src/nvml.h#L218-L241

The unversioned API (v1) and v2 API will return different results on R510+ drivers.

@jonathan-anyscale
decode pynvml bytes to str
1e0f5ad 
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
jjyao
jjyao approved these changes Nov 21, 2023
View reviewed changes
python/ray/_private/thirdparty/pynvml/__init__.py
# nvdia-ml-py version
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment saying why we pick this version: something like we are using this version because it uses v2 api and supports a wider range of drivers.

@jonathan-anyscale
nit
9a3332c 
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
@jonathan-anyscale
add pynvml version note
551c02d 
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
ericl
ericl approved these changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always great to see deps removed

@jjyao jjyao merged commit 9b9fb55 into ray-project:master Nov 22, 2023
2 checks passed
can-anyscale added a commit that referenced this pull request Nov 24, 2023
@can-anyscale
Revert "[Dashboard] Remove gpustats dependencies from Ray[default] (#…
513914c 
…41044)"

This reverts commit 9b9fb55.
@can-anyscale can-anyscale mentioned this pull request Nov 24, 2023
Merged
can-anyscale added a commit that referenced this pull request Nov 24, 2023
@can-anyscale
Revert "[Dashboard] Remove gpustats dependencies from Ray[default] (#…
1fbfa07 
…41044)" (#41375)

Reverts #41044

premerge is busted and potentially blocking people from merging into branch cut. Revert to unblock

Failing test: linux://python/ray/tests:test_streaming_generator_2

This reverts commit 9b9fb55.
@jonathan-anyscale jonathan-anyscale deleted the remove_gpustats branch November 27, 2023 17:29
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Nov 29, 2023
@jonathan-anyscale @ujjawal-khare
[Dashboard] Remove gpustats dependencies from Ray[default] (ray-proje…
108cf1d 
…ct#41044)

Add method to get gpu utilization similarly on how gpustats did, and remove gpustats from ray[default] dependencies.

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Nov 29, 2023
@can-anyscale @ujjawal-khare
Revert "[Dashboard] Remove gpustats dependencies from Ray[default] (r…
dff0310 
…ay-project#41044)" (ray-project#41375)

Reverts ray-project#41044

premerge is busted and potentially blocking people from merging into branch cut. Revert to unblock

Failing test: linux://python/ray/tests:test_streaming_generator_2

This reverts commit 9b9fb55.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuehaiPan XuehaiPan XuehaiPan left review comments

@wookayin wookayin wookayin approved these changes

@jjyao jjyao jjyao approved these changes

@ericl ericl ericl approved these changes

@richardliaw richardliaw Awaiting requested review from richardliaw richardliaw is a code owner

@edoakes edoakes Awaiting requested review from edoakes edoakes is a code owner

Assignees

@ericl ericl

@jjyao jjyao

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jonathan-anyscale @jjyao @ericl @wookayin @XuehaiPan